Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factoring out parts of the serve protocol for Hydra to share #6134

Merged
merged 5 commits into from
Jan 22, 2024

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Feb 20, 2022

Hydra uses the lock argument differently than Nix, so we un-hard-code it.

See NixOS/hydra#1165, which takes advantage of this (and #9560)

Depends on #9560

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be a very good reason that I’m missing, but wouldn’t it be simpler to directly use the legacy ssh store in hydra? That would also make it possible to incrementally make hydra store-agnostic by extending the store API to fit its needs

src/libstore/serve-protocol-impl.hh Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member Author

See what I wrote in NixOS/hydra#1165 (comment) I absolutely do want to use the Store abstraction long term, but I am very concerned with accidental breakage and so want to proceed quite slowly and incrementally to avoid it.

For example, without this approach, I probably would not have noticed how hydra is using a temp GC root locking version of queryPathInfo.

Once all the serialization logic is in Nix alone, making sure the relevant Stores expose enough stuff to obviate hydra using these "raw" serialization bits directly should a safer refactor, because all the shuffling happens in one repo, and none of the serialization logic itself need be touched.

@Ericson2314
Copy link
Member Author

CI is not working because recent changes broke the macOS / clang builds.

@thufschmitt
Copy link
Member

I’ve been looking into this a bit, and I wonder whether we couldn’t do things the other way around:

  1. Expose the LegacySSHStore in nix
  2. Make hydra use it for the connection (essentially replacing openConnection by the LegacySSHStore constructor)
  3. Gradually rewrite all the steps of the hydra build loop (the new functions from Split the buildRemote function hydra#1180) to use the store methods rather than directly writing/reading to from and to
  4. Make sure that hydra doesn’t use anything that’s not a generic Store interface and hide LegacySSHStore again

(all that while expanding the implementation of the store to cover the hydra-specific edge-cases, and adding the new methods needed for the stuff like non-blocking builds).

I think that it’s essentially similar to your plan, except that hydra would directly have a full-blown store available, which would make the incremental migration easier.

Do I miss anything?

@Ericson2314
Copy link
Member Author

@thufschmitt I am thinking start with #6223 and your Hydra PR, and then return to this. Sound good?

@Ericson2314 Ericson2314 marked this pull request as draft March 25, 2022 16:57
@stale stale bot added the stale label Oct 1, 2022
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Oct 25, 2023
@stale stale bot removed the stale label Dec 7, 2023
@Ericson2314 Ericson2314 force-pushed the expose-proto-rawer branch 2 times, most recently from 92fb541 to c114859 Compare December 7, 2023 18:15
@Ericson2314 Ericson2314 changed the title Start factoring out the serve protocol for Hydra to share Factoring out parts of the serve protocol for Hydra to share Dec 7, 2023
Ericson2314 added a commit to obsidiansystems/hydra that referenced this pull request Dec 7, 2023
Flake lock file updates:

• Updated input 'nix':
    'github:NixOS/nix/c3827ff6348a4d5199eaddf8dbc2ca2e2ef46ec5' (2023-12-07)
  → 'github:obsidiansystems/nix/c114859acb7180d1ef06571a1adca6130c30313e' (2023-12-07)
@Ericson2314
Copy link
Member Author

I fixed up the Hydra part, and tests are passing just great! (At one point was wrong and tests were failing, so we know the coverage is counting for something)

I thus consider this ready --- only keeping it draft because of the first Nix PR it depends on.

Copy link

dpulls bot commented Dec 8, 2023

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 marked this pull request as ready for review December 8, 2023 16:34
@Ericson2314 Ericson2314 changed the base branch from master to 2.19-maintenance December 8, 2023 16:34
@github-actions github-actions bot added documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Dec 8, 2023
@tomberek tomberek added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Dec 11, 2023
@Ericson2314
Copy link
Member Author

team meeting notes:

  • The "slow incremental migration" process is approved by the team (i.e. doing all these steps). That is on me torturing myself with many intermediate steps, shouldn't burden the team. When everything is done, and hydra isn't even using the legacy protocol any more, we should arrive at the same destination as if we did the "fast jump migration" process.

  • Still blocked on @thufschmitt taking a look because his concerns from a year ago that may or may not apply.

@Ericson2314
Copy link
Member Author

OK now this is rebased on top of the #9586 so we have the option of making these things part of LegacySSHStore for Hydra if @thufschmitt prefers!

Ericson2314 added a commit to obsidiansystems/hydra that referenced this pull request Dec 11, 2023
Flake lock file updates:

• Updated input 'nix':
    'github:NixOS/nix/c3827ff6348a4d5199eaddf8dbc2ca2e2ef46ec5' (2023-12-07)
  → 'github:obsidiansystems/nix/...'
@thufschmitt thufschmitt removed the new-cli Relating to the "nix" command label Dec 20, 2023
@Ericson2314 Ericson2314 removed repl The Read Eval Print Loop, "nix repl" command and debugger with-tests Issues related to testing. PRs with tests have some priority labels Jan 15, 2024
@Ericson2314
Copy link
Member Author

(in/after the meeting @thufschmitt said he didn't care about reviewing this one in particular.)

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look equivalent. Will approve when this is tested.

src/libstore/legacy-ssh-store.cc Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member Author

OK added some tests!

src/libstore/serve-protocol-impl.hh Outdated Show resolved Hide resolved
src/libstore/serve-protocol-impl.hh Outdated Show resolved Hide resolved
src/libstore/serve-protocol-impl.hh Outdated Show resolved Hide resolved
src/libstore/serve-protocol-impl.hh Outdated Show resolved Hide resolved
src/libstore/serve-protocol-impl.hh Outdated Show resolved Hide resolved
tests/unit/libstore/serve-protocol.cc Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the expose-proto-rawer branch 2 times, most recently from 1980514 to 8916e53 Compare January 22, 2024 17:41
Ericson2314 and others added 5 commits January 22, 2024 12:43
Factor out `ServeProto::BasicClientConnection` for Hydra to share

- `queryValidPaths`: Hydra uses the lock argument differently than Nix,
  so we un-hard-code it.

- `buildDerivationRequest`: Just the request half, as Hydra does some
  things between requesting and responding.

Co-authored-by: Robert Hensing <[email protected]>
Broader error handling logic is more robust.
We'll need this for unit testing.

Co-authored-by: Robert Hensing <[email protected]>
@Ericson2314 Ericson2314 merged commit 71bf592 into NixOS:master Jan 22, 2024
8 checks passed
@Ericson2314 Ericson2314 deleted the expose-proto-rawer branch January 22, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation fetching Networking with the outside (non-Nix) world, input locking idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. store Issues and pull requests concerning the Nix store
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants